[JENKINS-34983] - Add pipeline support #12

basil · 2018-06-09T07:29:44Z

Problem

The Text Finder plugin doesn't support Pipeline. It also has an old parent POM.

Solution

Add Pipeline support and update the parent POM to the latest version.

Implementation

I started by updating the parent POM to the latest version and specifying a minimum Jenkins version that had decent Pipeline support. I also added structs to the dependency list (for using @Symbol) and added the Pipeline plugin dependencies required for testing. Updating to the latest parent POM necessitated adding <?jelly escape-by-default='true'?> to the .jelly files.

To add Pipeline support to the plugin, I made it implement SimpleBuildStep as recommended in the documentation. As suggested in the "Constructor vs. setters" section of the documentation, I replaced the lengthy existing @DataBoundConstructor with a short one taking just truly mandatory parameters. For all optional parameters, I created a public setter marked with @DataBoundSetter (with any non-null default value set in the constructor or field initializer). This allows most parameters to be left at their default values in a Pipeline script, not to mention simplifying ongoing code maintenance because it is much easier to introduce new options this way. For Java-level compatibility, I left the previous constructor in place, but marked it @Deprecated. I also removed @DataBoundConstructor from it (there can be only one). Finally, I added a textFinder symbol (with @Symbol) to make use of this plugin from Pipeline more elegant.

Bumping the minimum Jenkins version resulted in a compilation error in FileCallable, so I replaced this with a call to the newer MasterToSlaveFileCallable class. I also extracted this callable into a static class to prevent warnings about Remoting serialization of anonymous inner classes. This necessitated marking a few methods called by the Callable as static methods.

Testing

This plugin had no tests, so I wrote some tests for both Freestyle and Pipeline jobs. The tests get about 60% coverage of the plugin. I got coverage of all the positive code paths and some (but not all) of the error conditions.

basil · 2018-06-09T07:37:07Z

It's unclear to me who should review/merge/release this. I note that there were previous pull requests to add Pipeline support to this plugin in 2016 and 2017. Maybe third time's a charm? Here's hoping for 2018. Paging @jglick since he was the last person to release an update of this plugin.

MarkEWaite · 2018-06-12T18:51:43Z

Nice work! Thanks for the improvements.

Can you explain in more detail why Pipeline support should be added to the text finder plugin when logfile reading is already available from existing Pipeline plugins?

Refer to the logContains usage and the assertContains implementation which uses the manager object from the Groovy post build plugin. Are you unwilling to use the Groovy post build plugin, or is there some other reason?

slide · 2018-06-12T20:26:44Z

The code looks good to me. You've done a nice job of documenting your changes and improving things. I second @MarkEWaite's question about the logContains/assertContains. Do those not do what you need?

basil · 2018-06-12T22:58:31Z

Nice work! Thanks for the improvements.

You're welcome!

Can you explain in more detail why Pipeline support should be added to the text finder plugin when logfile reading is already available from existing Pipeline plugins?

Pipeline support should be added to Text Finder to enable a smooth transition from Freestyle to Pipeline for legacy jobs without incurring the additional risk of regressions due to a change in the underlying implementation of the job as a result of using a different plugin. The Text Finder plugin also has the advantage of working for jobs that don't have the Groovy Sandbox enabled. While brand new Pipeline jobs are probably already following best practices and using the sandbox, this may not be the case for legacy jobs. As such, adding Pipeline support to this plugin would be convenient for users who want to migrate legacy code to Pipeline with as few changes as possible to the underlying job configuration and implementation.

MarkEWaite · 2018-06-12T23:21:07Z

As a specific comment to:

The Text Finder plugin also has the advantage of working for jobs that don't have the Groovy Sandbox enabled.

I'm not sure I understand what you mean by that. A blog post from Tyler Croy strongly recommends to not disable the Groovy Sandbox. I don't disable the Groovy Sandbox and I use the Groovy Post Build Plugin throughout my Jenkins installation.

I thought the default is that the Groovy Sandbox is enabled and an administrator must make a conscious (and dangerous) choice to disable it.

basil · 2018-06-12T23:35:17Z

I thought the default is that the Groovy Sandbox is enabled and an administrator must make a conscious (and dangerous) choice to disable it.

The administrator may want to consciously disable the Grooxy Sandbox and still use a plugin to analyze the build logs. The Text Finder plugin supports this use case, while the Groovy post build plugin does not. Hence this is another reason to port Text Finder to Pipeline.

oleg-nenashev

TaskListener serialization over Remoting is something I would recommend to avoid (in the context of JEP-200 & Co). I would recommend to use RemoteOutputStream and to create some auto tests which verify the behavior on agents

The rest of the code looks great.

oleg-nenashev · 2018-06-13T00:18:53Z

pom.xml

@@ -4,14 +4,19 @@
  <parent>
    <groupId>org.jenkins-ci.plugins</groupId>
    <artifactId>plugin</artifactId>
-    <version>1.480</version>
+    <version>3.4</version>


Actually, 3.15 now ;-)

oleg-nenashev · 2018-06-13T00:20:59Z

src/main/java/hudson/plugins/textfinder/TextFinderPublisher.java

@@ -201,8 +215,10 @@ private Pattern compilePattern(PrintStream logger) {
        return pattern;
    }

+    @Symbol("textFinder")


Maybe findText() to make it look like method. No strong preference

I've changed the symbol to findText as you requested.

oleg-nenashev · 2018-06-13T00:23:00Z

src/main/java/hudson/plugins/textfinder/TextFinderPublisher.java

@@ -233,6 +250,61 @@ public FormValidation doCheckRegexp(@QueryParameter String value) throws IOExcep
        }
    }

+    private static class FileChecker extends MasterToSlaveFileCallable<Boolean> {
+
+        private final TaskListener listener;


I would recommend to pass RemoteOutputStream there.
TaskListener gets here from API, and there is no guarantee that it will be always serializable over Remoting if somebody uses API

I changed {{TaskListener}} to {{RemoteOutputStream}} as you requested.

basil · 2018-06-13T00:52:34Z

I would recommend to use RemoteOutputStream and to create some auto tests which verify the behavior on agents

I created automated tests that verify the behavior on an agent as you requested.

slide · 2018-06-13T14:33:53Z

👍

oleg-nenashev · 2018-06-13T15:55:52Z

Jenkinsfile

@@ -0,0 +1 @@
+buildPlugin()


You can also set up testing against the 1.121.x branch so that we discover compatibility issues

buildPlugin(jenkinsVersions: [null, '2.121.1'])

oleg-nenashev

Looks good!

oleg-nenashev · 2018-06-13T15:57:43Z

@jglick Do you still maintain the plugin? If yes, would you be able to spin the release? If no, would you be fine if it is marked for adoption?

…ty issues

jglick · 2018-06-13T19:15:21Z

Do you still maintain the plugin?

No. Apparently I cut a release four and a half years ago, of which I have no memory.

would you be fine if it is marked for adoption?

Go ahead.

jglick

I am afraid I lack time to review miscellaneous conversions of plugins to Pipeline compatibility.

On a general note, I doubt you really need Pipeline compatibility for it, or that you need a plugin like this at all—just include something in your main build tool to the effect of

if fgrep -rl TODO .
then
    echo 'Problems found! ^^^'
    exit 1
fi

You can also use a tiny bit of Groovy to set builds to unstable rather than failed, etc.

basil · 2018-06-13T19:33:43Z

On a general note, I doubt you really need Pipeline compatibility for it, or that you need a plugin like this at all

Oh, I agree. I don't think anyone who is writing a brand new Pipeline job would use this plugin. As I wrote above, my main use case is in performing a mechanical conversion of a set of legacy jobs with as few changes as possible. Once my legacy jobs are migrated to Pipeline, I'll start rewriting significant parts of them to use conventions like the one described above.

oleg-nenashev · 2018-06-13T21:15:58Z

This is https://issues.jenkins-ci.org/browse/JENKINS-34983 BTW

@basil Would you be interested to take ownership of the plugin? I have unassigned @jglick according to the discussion and marked the plugin for adoption, so the ownership can be transferred at any moment

basil · 2018-06-13T21:35:33Z

Would you be interested to take ownership of the plugin?

Sure, I'll take it. There aren't too many open bugs or pull requests, anyway.

oleg-nenashev · 2018-06-13T22:41:29Z

@basil great, granted you permissions.
In order to release the plugin, you will also need to update https://github.com/jenkins-infra/repository-permissions-updater/blob/master/permissions/plugin-text-finder.yml

Welcome aboard!

kapoorlakshya · 2019-02-08T00:24:33Z

@basil Will there be a release soon? Interested in using this plugin in my project :).

Also, it would be nice to see an example for pipeline usage in the README. Thank you!

Add pipeline support

118ceb7

oleg-nenashev reviewed Jun 13, 2018

View reviewed changes

Basil Crow added 4 commits June 12, 2018 17:29

Update POM version

8a3753a

textFinder => findText (per review feedback)

dcaeee1

Add automated test on agent

ad508d1

TaskListener => RemoteOutputStream (per review feedback)

fc58b29

Add Jenkinsfile for https://ci.jenkins.io/job/Plugins/

563aaa3

oleg-nenashev reviewed Jun 13, 2018

View reviewed changes

oleg-nenashev approved these changes Jun 13, 2018

View reviewed changes

oleg-nenashev requested a review from jglick June 13, 2018 15:56

Set up testing against the 1.121.x branch for discovering compatibili…

5906137

…ty issues

jglick reviewed Jun 13, 2018

View reviewed changes

oleg-nenashev changed the title ~~Add pipeline support~~ [JENKINS-34983] - Add pipeline support Jun 13, 2018

basil mentioned this pull request Jun 14, 2018

Edit permissions for Text Finder plugin jenkins-infra/repository-permissions-updater#733

Merged

4 tasks

basil added 2 commits June 15, 2018 07:59

Bump minimum Jenkins version to 1.651

73c6bb1

Downgrade minimum Jenkins version to 1.650

1bf70f4

This was referenced Jun 15, 2018

Add Jenkinsfile #13

Closed

Added pipeline support #10

Closed

Add support for Jenkins pipeline support #6

Closed

basil added 6 commits June 15, 2018 09:17

Fix FindBugs warnings

4f4eb26

Attempt to fix tests on Windows

1ff8ffd

Remove getters

3b31863

Attempt to fix tests on Windows (again)

4c89b0c

Another try at fixing tests on Windows

410dc7e

Another try at fixing tests on Windows

54f6a0c

basil merged commit 5c473a4 into jenkinsci:master Jun 15, 2018

This was referenced Jun 15, 2018

Fix Japanese translation #14

Merged

Better log output to console #5

Closed

AdamBrousseau mentioned this pull request Jun 20, 2018

Discussion on how a test build is determined to be pass/fail eclipse-openj9/openj9#658

Closed

basil mentioned this pull request Jul 5, 2018

Better log output to console #21

Merged

basil deleted the pipeline-support branch May 1, 2019 20:10

basil mentioned this pull request Jun 15, 2019

Multiple finders #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-34983] - Add pipeline support #12

[JENKINS-34983] - Add pipeline support #12

basil commented Jun 9, 2018

basil commented Jun 9, 2018

MarkEWaite commented Jun 12, 2018

slide commented Jun 12, 2018

basil commented Jun 12, 2018

MarkEWaite commented Jun 12, 2018

basil commented Jun 12, 2018

oleg-nenashev left a comment

oleg-nenashev Jun 13, 2018

basil Jun 13, 2018

oleg-nenashev Jun 13, 2018

basil Jun 13, 2018

oleg-nenashev Jun 13, 2018

basil Jun 13, 2018

basil commented Jun 13, 2018

slide commented Jun 13, 2018

oleg-nenashev Jun 13, 2018

basil Jun 13, 2018

oleg-nenashev left a comment

oleg-nenashev commented Jun 13, 2018

jglick commented Jun 13, 2018

jglick left a comment

basil commented Jun 13, 2018

oleg-nenashev commented Jun 13, 2018

basil commented Jun 13, 2018

oleg-nenashev commented Jun 13, 2018

kapoorlakshya commented Feb 8, 2019

		@@ -0,0 +1 @@
		buildPlugin()

[JENKINS-34983] - Add pipeline support #12

[JENKINS-34983] - Add pipeline support #12

Conversation

basil commented Jun 9, 2018

Problem

Solution

Implementation

Testing

basil commented Jun 9, 2018

MarkEWaite commented Jun 12, 2018

slide commented Jun 12, 2018

basil commented Jun 12, 2018

MarkEWaite commented Jun 12, 2018

basil commented Jun 12, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basil commented Jun 13, 2018

slide commented Jun 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Jun 13, 2018

jglick commented Jun 13, 2018

jglick left a comment

Choose a reason for hiding this comment

basil commented Jun 13, 2018

oleg-nenashev commented Jun 13, 2018

basil commented Jun 13, 2018

oleg-nenashev commented Jun 13, 2018

kapoorlakshya commented Feb 8, 2019